Initial implementation for Keysight QCS #944

sorewachigauyo · 2024-07-31T08:22:32Z

Trial implementation of the core functions. Testing is rough on my side since I do not currently have access to the instrument library on my development machine, so I have to test the code on the embedded controller.

Checklist:

codecov · 2024-07-31T08:24:47Z

Codecov Report

Attention: Patch coverage is 0% with 194 lines in your changes missing coverage. Please review.

Project coverage is 47.37%. Comparing base (d16f03f) to head (3a29ffa).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/qibolab/_core/instruments/keysight/qcs.py	0.00%	62 Missing ⚠️
src/qibolab/_core/instruments/keysight/pulse.py	0.00%	55 Missing ⚠️
src/qibolab/_core/instruments/keysight/sweep.py	0.00%	35 Missing ⚠️
src/qibolab/_core/instruments/keysight/results.py	0.00%	20 Missing ⚠️
...b/_core/instruments/keysight/components/configs.py	0.00%	8 Missing ⚠️
src/qibolab/_core/instruments/keysight/__init__.py	0.00%	6 Missing ⚠️
.../_core/instruments/keysight/components/__init__.py	0.00%	4 Missing ⚠️
src/qibolab/instruments/keysight_qcs.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #944      +/-   ##
==========================================
- Coverage   50.55%   47.37%   -3.19%     
==========================================
  Files          63       71       +8     
  Lines        2886     3080     +194     
==========================================
  Hits         1459     1459              
- Misses       1427     1621     +194

Flag	Coverage Δ
unittests	`47.37% <0.00%> (-3.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/qibolab/instruments/keysight_qcs.py

alecandido · 2024-08-05T13:41:39Z

@sorewachigauyo I previously rebased https://github.com/qiboteam/qibolab/tree/0.2 on current https://github.com/qiboteam/qibolab/tree/main, thus I also rebased your PR consequently.

The Rust workflow is failing for unknown reasons (nothing changed on our side, so the reason should be in some dependency, but it seems non-reproducible locally).
I already blocked the workflow run on push in #885. As soon as we'll merge, I will update this branch as well. For the time being, just ignore it, please.

P.S.: same for QuTiP, which is making the other workflows to fail as well (but we should merge the PR very soon)

sorewachigauyo · 2024-10-01T17:44:38Z

Initial testing with readout spectroscopy using runcard https://github.com/qiboteam/qibolab_platforms_nqch/tree/qibolab-0.2/iqm5q

import numpy as np
import matplotlib.pyplot as plt
from qibolab import (
    AcquisitionType,
    AveragingMode,
    Parameter,
    Sweeper,
    create_platform,
)

platform = create_platform("iqm5q")
platform.connect()

qubit = platform.qubits[4]
natives = platform.natives.single_qubit[4]
sequence = natives.MZ.create_sequence()

f0 = platform.config(qubit.probe).frequency  # center frequency
sweeper = Sweeper(
    parameter=Parameter.frequency,
    range=(f0 - 20e6, f0 + 100e6, 1e6),
    channels=[qubit.probe],
)

results = platform.execute([sequence],
                           [[sweeper]],
                           nshots=100,
                           relaxation_time=100e3,
                           averaging_mode=AveragingMode.SEQUENTIAL,
                           acquisition_type=AcquisitionType.INTEGRATION)
_, acq = next(iter(sequence.acquisitions))

# plot the results
signal = results[acq.id]
amplitudes = np.abs(signal[..., 0] + 1j * signal[..., 1])
frequencies = sweeper.values

plt.title("Resonator Spectroscopy")
plt.xlabel("Frequencies [GHz]")
plt.ylabel("Amplitudes [a.u.]")
plt.plot(frequencies / 1e9, amplitudes * 1e6)
plt.savefig("test.png")

alecandido

Sorry for the lengthy review 😅

src/qibolab/_core/instruments/qcs.py

alecandido · 2024-10-17T12:31:41Z

src/qibolab/_core/instruments/qcs.py

+            for channel_id, input_op in acquisitions:
+                channel = self.virtual_channel_map[channel_id]
+                if channel not in acquisition_map:
+                    acquisition_map[channel] = []
+                acquisition_map[channel].append(input_op)


In principle, we could even lift the construction of such a mapping to the PulseSequence itself (do you think it may be useful elsewhere? @stavros11)

Of course, it would be a ChannelId -> [InputOps] mapping, that you could supplement with your ChannelId -> qcs.Channels mapping to obtain the desired result.

src/qibolab/_core/instruments/qcs.py

alecandido

Thanks @sorewachigauyo, now it seems extremely cleaner.

I'm sorry that a bunch of functions right now require a lot of arguments. We could try to find a better way to split some parts, partially evaluate tasks, or better package the arguments.
But this is really the kind of refactor that can be postponed (almost forever...).

Most of the comments are on the sweepers, which is frequently the most critical part. All the others seem pretty nice (though I may spend a bit more time later on proposing a small refactor about the array manipulations in the result - but nothing that serious, it's already pretty succinct).

src/qibolab/_core/instruments/keysight/sweep.py

alecandido · 2024-10-19T06:30:07Z

src/qibolab/_core/instruments/keysight/sweep.py

+        for idx2, sweeper in enumerate(parallel_sweeper):
+            qcs_variable = qcs.Scalar(
+                name=f"V{idx}_{idx2}", value=sweeper.values[0], dtype=float
+            )
+
+            if sweeper.parameter is Parameter.frequency:
+                sweeper_channel_map.update(
+                    {channel_id: qcs_variable for channel_id in sweeper.channels}
+                )


Sorry @sorewachigauyo, this is not about this PR, but rather Qibolab's overall design. I'm writing this here because it is an optimal example to discuss, and you're invited to provide your opinion as well. I may lift this immediately as a separate issue, I'm just undecided.

With @stavros11, we wondered at some point whether to strip away the option of having multiple channels and pulses swept by a single sweeper (not a parallel one). In fact, this could be now replaced by an equivalent set of parallel sweepers.
We didn't do that because of the specific implementation of parallel sweepers in QM, which requires the usage of a for_each_ context manager, which had some disadvantages with respect to a plain for_.

It now appears to me that quite redundant, since here you have explicitly three nested iterations:

idx about the sweepers' group

idx2 within the parallel sweepers

and sweeper.channels (or sweeper.pulses down below - exact analogue)

These could have been decreased by one level by unraveling the .channels in the parallel group.

However, it is true that, thanks to the presence of multiple .channels, you are able to use a single qcs_variable, and this could be an advantage, and a reason to keep the current behavior (which we would keep anyhow, but we may consider deprecating at some point - and finally removing in 0.3).

What do you think?

I don't have a strong opinion of the subject as I can go either way and have been hacking the driver implementation to fit the ongoing design.

Though, since Qibolab 0.3 is targeting program-level control, it may be possible to pass that program directly to a FPGA without much conversion/parsing layers in the driver. Also, unraveling the channel/pulse arrays could help in maybe a Qibolab FPGA implementation by reducing the complexity required to implement sweepers.

Just to clarify: Qibolab itself is not planning to become an FPGA design, but the idea is to have a high-level language to be compiled to the individual ones. But at the same time, I'd like to keep it as close as possible to the existing languages to minimize the conversion (i.e. compilation) efforts.

If then you're willing to contribute to the language design, and you may want to use it as the basis to design an FPGA-based processor (at which point, it would require essentially no conversion), you're definitely more than welcome :P

src/qibolab/_core/instruments/keysight/sweep.py

src/qibolab/_core/instruments/keysight/results.py

alecandido · 2024-10-22T17:17:54Z

Personally, I prefer stricter separation, like in C++ namespaces and anonymous namespaces.

I like that as well, e.g. the strictly modular structure in Rust, in which you need to declare any visibility beyond the current namespace.

However, that's not Python style, and even __all__ is not saving you from there, since you can always directly import (__all__ has only an effect on the from mymod import * statements, and nothing else).
That's why I added the _core, and separately re-exporting top-level

The Python philosophy is to leave everything public, since

we're all consenting adults here

(not able to find a proper reference, sorry, but it is a GvR quote)

The only way to go beyond that are naming conventions, and I'd suggest avoiding double leading underscores, unless you're extremely confident (but it's unfamiliar and a bit confusing...).

So, whatever is inside qibolab._core is already internal and not exposed to the end user.
If you want identifiers to be scoped within a module, prepend them with a leading underscore. If you want an entire module to be scoped within a subpackage, prepend its name with a leading underscore.

sorewachigauyo · 2024-10-23T07:31:42Z

Tested some 2D scans, so sweepers should be good to go

alecandido

Another tiny proposal for the results handling, but other than that it looks good to me.

I'll wait for @stavros11 review, but it may be good to go :)

Thanks @sorewachigauyo for your effort!

src/qibolab/_core/instruments/keysight/results.py

stavros11

Thanks @sorewachigauyo for this. I left some comments below, most are not very important and mainly cosmetics, I would approve immediately if it weren't for the last two comments (mainly the last one), which I consider the most important. The main issue with these is that they are interface facing, and it would be good to address here in order to avoid breaking later.

In any case, as I mentioned also in the comments, I am have never used these instruments in practice, so if you prefer to keep the existing approach it is also fine with me. I am more concerned with avoid breaking in the future, than the exact approach used.

Also, I have not read all resolved comments from the previous reviews as I was not following last week, so I am sorry if something was already discussed before.

stavros11 · 2024-10-29T12:14:51Z

src/qibolab/_core/instruments/keysight/pulse.py

+    elif isinstance(shape, (Gaussian, Drag)):
+        return qcs.GaussianEnvelope(shape.rel_sigma)


What is the motivation to include Drag here? It seems that this always returns a Gaussian, completely ignoring the DRAG beta parameter. If DRAG is not available then doesn't it make more sense to just raise an error, like for all other shapes?

So the DRAG is implemented here at https://github.com/qiboteam/qibolab/pull/944/files#diff-e3c18ab9440f0e68561a15f5e9497097a405ef090fd2612f4f3a362c71c83502R126-R127
and I need the gaussian as the base envelope for the summation of the base and derivative envelope.
(https://quantumbenchmark.com/docs/qcs/api/channels.html#rj-drag)

I could implement the DRAG based on Qibolab's implementation, but I would not like to create a static envelope especially when duration sweepers can change the envelope size.

@sorewachigauyo I understand your concern, and now that you explained I'm even fine with your solution.

Not to enforce the change request, but as a general note, we suspect that most of the APIs are actually implementing duration sweepers in the same inefficient way you would do using Qibolab's envelopes (i.e. evaluating on software all the required arrays and uploading them).
This is not happening for QM, but there is a dedicated feature linked to that, and we're now exposing it as a separate sweeper (where the envelope is not uploaded multiple times, but interpolated on the device).

qibolab/src/qibolab/_core/sweeper.py

Line 25 in f74c334

duration_interpolated = (auto(), _PULSE)

If you suspect Keysight to be able to interpolate as well, I'd recommend you to verify it (as much as you can), and expose it through that sweeper, instead of the bare duration one (there will be a fallback at some level, but they may have different caveats, related to timing and approximation).

src/qibolab/_core/instruments/keysight/qcs.py

src/qibolab/_core/instruments/keysight/results.py

stavros11 · 2024-10-29T12:43:38Z

src/qibolab/_core/instruments/keysight/sweep.py

+                # Duration is not supported with hardware sweeping
+                if sweeper.parameter is Parameter.duration:
+                    hardware_sweeping = False


I'd guess that it should be possible to support this by uploading multiple waveforms, which is the default solution we are using for QM and I believe we will also use in Qblox and others.

It is fine to not do this in this PR. If you are interested in doing it some time in the future, you can open an issue for it.

I am told duration sweeping will eventually be supported, so I think this is fine for now to make the backend layer handle it in software (which I assume is exactly the implementation you are describing right now, just not explicit).

Are you sure?
The implementation @stavros11 is talking about is inefficient, since it requires multiple arrays uploads, but not as inefficient as what we usually call "a software sweeper", since the multiple arrays will be uploaded on the device, and they will be executed without any networking, nor any memory exchange on the device itself. While a software sweeper (for us) is just a software loop, repeating many separate experiments and downloading the results at each iteration

stavros11 · 2024-10-29T12:45:27Z

src/qibolab/_core/instruments/keysight/sweep.py

+        # Currently nested hardware sweeping is not supported
+        hardware_sweeping = len(hardware_sweepers + software_sweepers) == 0


This can also be an issue, as it may be useful for various routines, in terms of performance.

I am told its on the roadmap.

src/qibolab/_core/instruments/keysight/qcs.py

stavros11 · 2024-10-29T13:35:39Z

src/qibolab/_core/instruments/keysight/qcs.py

+    # Map of QCS virtual channels to QCS physical channels
+    qcs_channel_map: qcs.ChannelMapper
+    # Map of Qibolab channel IDs to QCS virtual channels
+    virtual_channel_map: dict[ChannelId, qcs.Channels]


Looking at the example platform here(*): https://github.com/qiboteam/qibolab_platforms_nqch/blob/qibolab-0.2/iqm5q/platform.py, I have the impression that this dictionary can be eliminated by using qibolab's channel.path, which I believe is currently not used. I am not sure if types can be worked out though, because our channel.path should be a str. If qcs.ChannelMapper provides string keys, you may be able to retain the information using just qcs_channel_map and channel.path, otherwise it may be tricky.

In any case, I have no experience with these particular instruments, so if you find it easier to use it this way, it is fine with me.

(*) Unrelated to the above, it would be useful to also have an example (not necessarily real/working) platform here, but we can postpone this for later, when we have better testing or even better documentation for how to use each driver.

I prefer to have a direct mapping because other approaches would involve some type casting or eval to get the necessary parameters from the channel object and start to warp the model.

The qcs.ChannelMapper does not refer to channels by string keys but offers an array view to the virtual channel bundles.

src/qibolab/_core/instruments/keysight/qcs.py

stavros11

Thanks for the updates and responses @sorewachigauyo. Other than the unresolved discussions above, mainly about the duration sweep and generally software vs hardware sweeps, which I guess that we all agree we can postpone for later, this looks good to me for merging.

It would be good if you can test on the actual device before merging this, as we cannot do that on our side. My main concern is the last change adding the QcsAcquisitionConfig, which looks good in terms for code, however I saw that the platform in the other repo has not been updated to use it and we should probably verify if it works.

alecandido · 2024-11-02T12:42:14Z

Thanks @stavros11 for the review!

It would be good if you can test on the actual device before merging this, as we cannot do that on our side. My main concern is the last change adding the QcsAcquisitionConfig, which looks good in terms for code, however I saw that the platform in the other repo has not been updated to use it and we should probably verify if it works.

@sorewachigauyo, I guess it would be ideal if you can just repeat a whatever scan, or even publish a report with a few routines from qiboteam/qibocal#990

Other than that, I would aim to merge asap, to get further traction for 0.2.

sorewachigauyo · 2024-11-04T07:12:34Z

It would be good if you can test on the actual device before merging this, as we cannot do that on our side. My main concern is the last change adding the QcsAcquisitionConfig, which looks good in terms for code, however I saw that the platform in the other repo has not been updated to use it and we should probably verify if it works.

I have tested this on my machine.

@sorewachigauyo, I guess it would be ideal if you can just repeat a whatever scan, or even publish a report with a few routines from qiboteam/qibocal#990

Unfortunately, it would be next week earliest as we are currently installing the full control electronics package and other maintenance works on the fridge.

alecandido reviewed Jul 31, 2024

View reviewed changes

src/qibolab/instruments/keysight_qcs.py Outdated Show resolved Hide resolved

alecandido force-pushed the 0.2 branch from f491983 to 8f58317 Compare August 5, 2024 10:32

alecandido force-pushed the keysight-qcs branch from 3de5459 to 1a4d052 Compare August 5, 2024 13:39

alecandido force-pushed the 0.2 branch from 52c0862 to 5d94231 Compare August 16, 2024 14:46

Base automatically changed from 0.2 to main September 20, 2024 12:05

sorewachigauyo force-pushed the keysight-qcs branch from c14a227 to 41dd538 Compare October 1, 2024 14:03

sorewachigauyo marked this pull request as ready for review October 16, 2024 15:32

sorewachigauyo requested review from alecandido and stavros11 October 16, 2024 15:32

alecandido reviewed Oct 17, 2024

View reviewed changes

alecandido reviewed Oct 19, 2024

View reviewed changes

sorewachigauyo force-pushed the keysight-qcs branch from 9b5f6f8 to 11ba3cb Compare October 23, 2024 02:16

alecandido approved these changes Oct 23, 2024

View reviewed changes

src/qibolab/_core/instruments/keysight/results.py Outdated Show resolved Hide resolved

sorewachigauyo added 12 commits October 28, 2024 14:41

Initial implementation

7d70d33

fix: Fix sweeper implementation

4bca03f

fix: Linter fixes

79ef94f

fix: sweeper and result format fixes

a5fc2fb

perf: delegate virtual z to hardware

145db4e

style: explicit nanosecond unit conversion

7f72d61

fix: fix delay implementation for flux pulses

ecb78cd

fix: fix flux pulses being ignored

3330088

refactor: define sampling rate as class variable

a0110c1

refactor: use defaultdict for acquisiton map

e1bcbf7

feat: expose driver in instruments

03e3498

perf: optimize nested sweepers to use hardware if possible

6e49f85

sorewachigauyo added 14 commits October 28, 2024 14:41

refactor: sweeper management

64e897a

refactor: move sweeper and pulses to seperate modules

9bad5fb

refactor: Move result handling to side module

9b6005a

refactor: inline RFWaveform creation

69bc929

perf: rely on hardware for state determination

fd29be6

refactor: move result parsing to side module

c29d344

fix: fix waveform conversion

78f695b

refactor: module level imports and exports

2511e0e

docs: fix comments

e92da05

refactor: define supported sweepers as top level constant

11a7b72

refactor: remove sweeper swaps

5140abd

fix: fix result parsing bug

38fc84d

fix: fix sweepers

c0b9564

refactor: clean up result management

3a48f83

alecandido force-pushed the keysight-qcs branch from 5e490c1 to 3a48f83 Compare October 28, 2024 13:41

stavros11 reviewed Oct 29, 2024

View reviewed changes

sorewachigauyo added 4 commits November 1, 2024 14:52

refactor: remove unused

50de1d0

docs: field docstrings

402d266

refactor: inline qcs program initialization

8c279d1

feat: add config for qcs classifier

3a29ffa

stavros11 approved these changes Nov 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation for Keysight QCS #944

Initial implementation for Keysight QCS #944

sorewachigauyo commented Jul 31, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading

alecandido commented Aug 5, 2024 •

edited

Loading

sorewachigauyo commented Oct 1, 2024 •

edited

Loading

alecandido left a comment

alecandido Oct 17, 2024

alecandido left a comment

alecandido Oct 19, 2024

sorewachigauyo Oct 29, 2024

alecandido Oct 29, 2024 •

edited

Loading

alecandido commented Oct 22, 2024

sorewachigauyo commented Oct 23, 2024

alecandido left a comment

stavros11 left a comment

stavros11 Oct 29, 2024

sorewachigauyo Oct 29, 2024

alecandido Oct 29, 2024

stavros11 Oct 29, 2024

sorewachigauyo Oct 29, 2024

alecandido Oct 29, 2024

stavros11 Oct 29, 2024

sorewachigauyo Oct 29, 2024

stavros11 Oct 29, 2024

sorewachigauyo Nov 1, 2024 •

edited

Loading

stavros11 left a comment

alecandido commented Nov 2, 2024

sorewachigauyo commented Nov 4, 2024

		elif isinstance(shape, (Gaussian, Drag)):
		return qcs.GaussianEnvelope(shape.rel_sigma)

		# Currently nested hardware sweeping is not supported
		hardware_sweeping = len(hardware_sweepers + software_sweepers) == 0

Initial implementation for Keysight QCS #944

Are you sure you want to change the base?

Initial implementation for Keysight QCS #944

Conversation

sorewachigauyo commented Jul 31, 2024 • edited Loading

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

alecandido commented Aug 5, 2024 • edited Loading

sorewachigauyo commented Oct 1, 2024 • edited Loading

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

alecandido commented Oct 22, 2024

sorewachigauyo commented Oct 23, 2024

alecandido left a comment

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorewachigauyo Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

alecandido commented Nov 2, 2024

sorewachigauyo commented Nov 4, 2024

sorewachigauyo commented Jul 31, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading

alecandido commented Aug 5, 2024 •

edited

Loading

sorewachigauyo commented Oct 1, 2024 •

edited

Loading

alecandido Oct 29, 2024 •

edited

Loading

sorewachigauyo Nov 1, 2024 •

edited

Loading